Open Service: Fix reactivity on deep signals, fire subscribers on load dependencies#34979
Conversation
…on re-emit Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
Re-engage alien-signals' identity-based dedup in subscribeToQuery by memoizing the computed: return the previous reference when the new handler output is deeply equal. Output-schema validation and Immer both mint fresh references for unchanged data, which previously re-fired subscribers on every state write (including writes to unrelated keys and loads that rewrite a deeply-equal payload). Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
…te-only Replace the single alien-signals state atom + Immer with a deepsignal deep reactive proxy (backed by @preact/signals-core). State mutates in place inside a batch; subscriptions track per-field reads so an unrelated key/field write never re-runs a subscriber's handler. Schemas now validate shape only and never convert: validation output is discarded and the original reference is returned. A dev-only check throws OpenServiceSchemaConversionError when a schema transforms/strips/coerces. subscribe() gains an optional selector (universal-store pattern): the callback receives the selected slice and fires only when it changes by value. Emissions are deduped by value via es-toolkit isEqual; query results and emissions are detached plain snapshots so the proxy never escapes the runtime.
Output validation now runs only on pull boundaries (query()/.loaded()/ static build) and on whole-value subscription emissions, never narrowing a selector subscriber's reactive footprint. Because validation no longer runs where it would broaden tracking, schemas may transform/coerce again: reverted the no-conversion contract and removed OpenServiceSchemaConversionError. With a selector, the computed reads only the selected slice, so an unselected sibling-field change no longer re-runs the handler (regression guard added). Snapshotting: structuredClone for the plain whole-state snapshot; JSON only to strip live proxy slices for selectors. Renamed the docgen-flavored test fixture to concept-based naming (rebuiltEqualValueOnLoadServiceDef) to match the existing convention.
…es & tests - Selector subscriptions now validate output too: validation runs untracked so it catches shape errors without broadening the reactive footprint (regression-guarded). Fixes output validation being skipped when a selector was passed. - createServiceRuntime no longer clones initialState; the clone moves to registerService (the static build already clones), removing a double clone while still protecting a definition's shared initialState. - Remove the false-positive 'different record' subscription test (covered better by the handler-spy fine-grained test) and merge the two selector tests into one emit+recompute test. - Drop conversion framing from README/JSDoc; validation is described as it always behaved.
The draft naming was an Immer-ism; with the deep-signal proxy the callback receives the live state, so name it accordingly across the type, fixtures, docgen service, debug service, tests, and README.
A query's load now re-fires for active subscriptions whenever the external signals it reads synchronously change (same-service or cross-service via getService), turning it into a reactive async resource. Implemented by running the load inside its own effect() so its reads are ambiently tracked; loads with no external synchronous reads still fire exactly once, so existing services are unaffected. Superseding: each run carries an epoch and writes through epoch-gated commands, so a slow stale load cannot overwrite a newer run's state. Coalescing: changes batched together produce a single re-load. Direct query()/.loaded() calls keep one-shot semantics. The load effect is torn down with the subscription. Loads are now contractually idempotent warming steps; one-shot side effects belong in commands.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors the open-service state management system from alien-signals to deepsignal-backed reactivity. Core changes include replacing ChangesOpen Service Deep Reactivity and State Refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/shared/open-service/service-runtime.test.ts`:
- Line 327: Replace fixed sleeps (e.g., the await new Promise(resolve =>
setTimeout(resolve, 50)) calls in service-runtime.test.ts) with deterministic
waits using the test runner's wait utilities (vi.waitFor or similar) that wait
for a concrete condition: check a spy/called count, assert the expected
length/content of the emitted array, or wait for a specific state change. Locate
the instances in the tests where those setTimeout sleeps are used and swap them
for vi.waitFor(() => expect(spy).toHaveBeenCalledTimes(n)) or vi.waitFor(() =>
expect(emitted).toEqual([...])) so the test completes when the observable/spy
reaches the expected state rather than after a fixed time.
In `@code/core/src/shared/open-service/service-runtime.ts`:
- Around line 1024-1027: The selector branch currently calls selector(output) on
raw handler output, bypassing schema validation/coercion; change it to run
validateQueryOutput(refs, queryName, queryDef, output) inside the existing
untracked call, capture the validated/coerced value (e.g., const validated =
validateQueryOutput(...)), then call selector(validated) and return
detachSnapshot(selector(validated)). Keep the untracked wrapper and existing
references (selector, untracked, validateQueryOutput, detachSnapshot, output,
queryName, queryDef, refs) so selector subscribers receive the same
validated/coerced TOutput as query()/ .loaded().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 436b89f1-058f-47ba-a54b-14de09619f8a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
code/.storybook/open-service-debug-service.tscode/core/package.jsoncode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/fixtures.tscode/core/src/shared/open-service/index.test-d.tscode/core/src/shared/open-service/server.test-d.tscode/core/src/shared/open-service/server.test.tscode/core/src/shared/open-service/server.tscode/core/src/shared/open-service/service-registration.test.tscode/core/src/shared/open-service/service-registration.tscode/core/src/shared/open-service/service-runtime.test.tscode/core/src/shared/open-service/service-runtime.tscode/core/src/shared/open-service/services/docgen/server.tscode/core/src/shared/open-service/types.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 74 | 🚨 +2 🚨 |
| Self size | 20.42 MB | 20.40 MB | 🎉 -25 KB 🎉 |
| Dependency size | 36.11 MB | 36.65 MB | 🚨 +539 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 205 | 🚨 +2 🚨 |
| Self size | 908 KB | 908 KB | 0 B |
| Dependency size | 88.61 MB | 89.13 MB | 🚨 +513 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 198 | 🚨 +2 🚨 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 87.10 MB | 87.62 MB | 🚨 +513 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 75 | 🚨 +2 🚨 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 56.53 MB | 57.05 MB | 🚨 +513 KB 🚨 |
| Bundle Size Analyzer | node | node |
…orybookjs/storybook into jeppe-cursor/docgen-subscription-referential-equality-5a81 # Conflicts: # code/core/src/shared/open-service/service-runtime.test.ts # code/core/src/shared/open-service/service-runtime.ts
Selector subscribers now receive schema-validated slices. A tracked selector(output) read preserves fine-grained proxy dependencies because validation returns a plain parsed value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
code/core/src/shared/open-service/README.md (1)
382-382: ⚡ Quick winAdd language specifiers to fenced code blocks.
Two fenced code blocks are missing language specifiers. Add identifiers to improve markdown rendering and enable syntax highlighting where applicable.
📝 Proposed fix
Line 382 (state sync diagram):
-``` +```text ┌─────────────────────────┐ channel (services:*) ┌─────────────────────────┐ │ Manager process │ ◄────────────────────────► │ Preview process │Line 429 (state sync sequence):
-``` +```text Peer A (manager) Channel Peer B (preview) ─────────────────────────────────────────────────────────────────Also applies to: 429-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/shared/open-service/README.md` at line 382, Add language specifiers to the two fenced code blocks in the README: the "state sync diagram" block that begins with the Manager/Preview ASCII diagram and the "state sync sequence" block that begins with "Peer A (manager) Channel Peer B (preview)"; update their opening ``` fences to include a language identifier (e.g., ```text or ```diff) so Markdown renderers apply syntax highlighting and preserve formatting.code/e2e-tests/open-service-background.spec.ts (1)
36-55: ⚡ Quick winPrefer web-first waits over fixed
waitForTimeout(5_000).The two hard 5s sleeps are a Playwright anti-pattern: they add unconditional latency yet can still race on slow CI. The
expect(darkSwatch).toBeVisible()and.poll(...)calls already wait deterministically, so the explicit sleeps can be dropped (or replaced with a concrete readiness condition such as waiting for the iframeload/ a preview element).♻️ Proposed change
await page.goto(`${storybookUrl}/?path=${STORY_PATH}`); await page.waitForSelector('`#storybook-preview-iframe`'); - await page.waitForTimeout(5_000); const darkSwatch = page.getByRole('toolbar').getByRole('button', { name: 'Dark', exact: true }); await expect(darkSwatch).toBeVisible(); await darkSwatch.click(); @@ await page.reload(); await page.waitForSelector('`#storybook-preview-iframe`'); - await page.waitForTimeout(5_000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/e2e-tests/open-service-background.spec.ts` around lines 36 - 55, Remove the two fixed page.waitForTimeout(5_000) sleeps and replace them with web-first waits: after goto and after page.reload, wait for the preview iframe to finish loading (e.g., await page.frameLocator('`#storybook-preview-iframe`').locator('body').waitFor() or use an explicit wait for the previewBodyBackground readiness) and then reuse the existing expect.poll(() => previewBodyBackground(page), ...) checks; keep the darkSwatch visibility/assertions and ensure previewBodyBackground is polled after the iframe load so the test no longer relies on hard sleeps.code/core/src/shared/open-service/service-sync.ts (1)
73-75: 💤 Low value
isPlainObjectmay incorrectly accept class instances or objects with modified prototypes.The check
typeof value === 'object' && value !== null && !Array.isArray(value)passes for class instances,Object.create(null)objects, and objects with non-Object prototypes. For untrusted channel payloads, consider checkingObject.getPrototypeOf(value) === Object.prototype || Object.getPrototypeOf(value) === nullto ensure only true plain objects are accepted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/shared/open-service/service-sync.ts` around lines 73 - 75, The current isPlainObject function accepts class instances and objects with modified prototypes; change its implementation to verify the object's prototype is either Object.prototype or null by using Object.getPrototypeOf(value) === Object.prototype || Object.getPrototypeOf(value) === null in addition to the existing typeof/value/null/Array checks so only true plain objects (including Object.create(null)) pass; keep the function name isPlainObject and its type predicate signature when updating the logic.code/core/src/channels/postmessage/index.ts (1)
233-239: 💤 Low valuePotential redundant flush when both paths trigger.
The new flush at line 238 doesn't check
this.connected, while the existing flush insetHandler(lines 53-56) setsconnected = trueto prevent re-flushing. IfsetHandler's flush already ran (settingconnected = true), this new flush will still execute on every incoming message whilebuffer.length > 0, thoughflush()itself clears the buffer so subsequent calls are no-ops.The logic is safe (buffer is cleared), but consider guarding with
!this.connectedfor clarity and to avoid the extra function call:- if (this.config.page === 'manager' && this.buffer.length) { + if (this.config.page === 'manager' && this.buffer.length && !this.connected) { this.flush(); + this.connected = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/channels/postmessage/index.ts` around lines 233 - 239, The preview postMessage flush can redundantly call flush() even after setHandler has already flushed and set this.connected; update the conditional in the postMessage handling block (the code that currently checks this.config.page === 'manager' && this.buffer.length) to also require !this.connected before calling this.flush(), so it only flushes when the channel hasn't already been marked connected by the setHandler path (referencing this.connected, setHandler, and flush).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/.storybook/services-preset.ts`:
- Around line 6-12: Update the file-level comment above the services preset to
stop implying the background example is gated by STORYBOOK_OPEN_SERVICE_DEBUG;
state that registerBackgroundService() runs unconditionally and only the debug
service is enabled by the STORYBOOK_OPEN_SERVICE_DEBUG flag (reference the
services function and the registerBackgroundService call), so readers know the
env var only controls the debug service.
---
Nitpick comments:
In `@code/core/src/channels/postmessage/index.ts`:
- Around line 233-239: The preview postMessage flush can redundantly call
flush() even after setHandler has already flushed and set this.connected; update
the conditional in the postMessage handling block (the code that currently
checks this.config.page === 'manager' && this.buffer.length) to also require
!this.connected before calling this.flush(), so it only flushes when the channel
hasn't already been marked connected by the setHandler path (referencing
this.connected, setHandler, and flush).
In `@code/core/src/shared/open-service/README.md`:
- Line 382: Add language specifiers to the two fenced code blocks in the README:
the "state sync diagram" block that begins with the Manager/Preview ASCII
diagram and the "state sync sequence" block that begins with "Peer A (manager)
Channel Peer B (preview)"; update their opening ``` fences to
include a language identifier (e.g., ```text or ```diff) so Markdown renderers
apply syntax highlighting and preserve formatting.
In `@code/core/src/shared/open-service/service-sync.ts`:
- Around line 73-75: The current isPlainObject function accepts class instances
and objects with modified prototypes; change its implementation to verify the
object's prototype is either Object.prototype or null by using
Object.getPrototypeOf(value) === Object.prototype ||
Object.getPrototypeOf(value) === null in addition to the existing
typeof/value/null/Array checks so only true plain objects (including
Object.create(null)) pass; keep the function name isPlainObject and its type
predicate signature when updating the logic.
In `@code/e2e-tests/open-service-background.spec.ts`:
- Around line 36-55: Remove the two fixed page.waitForTimeout(5_000) sleeps and
replace them with web-first waits: after goto and after page.reload, wait for
the preview iframe to finish loading (e.g., await
page.frameLocator('`#storybook-preview-iframe`').locator('body').waitFor() or use
an explicit wait for the previewBodyBackground readiness) and then reuse the
existing expect.poll(() => previewBodyBackground(page), ...) checks; keep the
darkSwatch visibility/assertions and ensure previewBodyBackground is polled
after the iframe load so the test no longer relies on hard sleeps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20900f7a-7e0c-4cbd-9ed4-24c412b10d38
📒 Files selected for processing (36)
code/.storybook/background-service/definition.tscode/.storybook/background-service/manager.tsxcode/.storybook/background-service/preview.tscode/.storybook/background-service/server.tscode/.storybook/manager.tsxcode/.storybook/preview.tsxcode/.storybook/services-preset.tscode/core/src/channels/postmessage/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/manager-api/modules/provider.tscode/core/src/manager-api/root.tsxcode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/client.tscode/core/src/shared/open-service/fixtures.tscode/core/src/shared/open-service/manager.tscode/core/src/shared/open-service/preview.tscode/core/src/shared/open-service/server.test-d.tscode/core/src/shared/open-service/service-channel.tscode/core/src/shared/open-service/service-client.test.tscode/core/src/shared/open-service/service-client.tscode/core/src/shared/open-service/service-definition.tscode/core/src/shared/open-service/service-registration-sync.test.tscode/core/src/shared/open-service/service-registration.test.tscode/core/src/shared/open-service/service-registration.tscode/core/src/shared/open-service/service-runtime.tscode/core/src/shared/open-service/service-sync.tscode/core/src/shared/open-service/service-transport.tscode/core/src/shared/open-service/services/docgen/definition.tscode/core/src/shared/open-service/services/docgen/server.tscode/core/src/shared/open-service/types.tscode/core/src/shared/open-service/use-service-command.test.tsxcode/core/src/shared/open-service/use-service-command.tscode/core/src/shared/open-service/use-service-query.test.tsxcode/core/src/shared/open-service/use-service-query.tscode/e2e-tests/open-service-background.spec.tscode/playwright.config.ts
✅ Files skipped from review due to trivial changes (3)
- code/.storybook/background-service/definition.ts
- code/.storybook/manager.tsx
- code/core/src/shared/open-service/client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/shared/open-service/service-runtime.ts
- code/core/src/shared/open-service/fixtures.ts
768479d to
8715ba8
Compare
Closes #
Works on #34824
What I did
Reworked the open-service reactive core to use per-field deep reactivity instead of one coarse state atom. This fixes docgen-style double emissions on re-navigation when data is unchanged, makes subscriptions precise, and makes a query's async
loadreactive to its synchronous dependencies.State and writes
alien-signalsatom + Immer withdeepSignal(deepsignal) backed by@preact/signals-core. Readingctx.self.state.<field>tracks only that field (including record keys added later).setState((state) => …)mutates the proxy in place insidebatch(); only changed fields invalidate dependents.Subscriptions
selectoronsubscribe(universal-store pattern):subscribe(input, selector, callback). The selector narrows reactive reads so unrelated sibling fields do not re-run the handler.es-toolkitisEqual. Emissions are detached plain values; whole-state snapshots usestructuredClone, with JSON detach only for proxy slices from selectors.Validation
query(),.loaded(), static build, subscription emit), not on the reactive path. Selector subscribers validate untracked so validation cannot broaden the tracked footprint.Reactive
loadloadre-runs when external signals it reads synchronously change (same-service fields or cross-service viagetService). Loads are idempotent warming steps; one-shot side effects belong in commands. Superseded runs are epoch-gated so stale writes cannot clobber newer results. Directquery()/.loaded()remain one-shot.Dependencies
alien-signals; addeddeepsignaland@preact/signals-core.Demonstrated behavior (docgen-style emulation: provider returns fresh-but-equal payloads on navigation):
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
No manual testing is required. Changes are confined to the internal open-service runtime under
code/core/src/shared/open-service/. Coverage is in the Vitest suite (yarn test open-service), including fine-grained handler-spy tests, selector tests, equal-value dedup, and reactive load scenarios (same-service and cross-service deps, coalescing, superseding in-flight loads, non-subscriptionquery()one-shot behavior).Documentation
MIGRATION.MD
Updated
code/core/src/shared/open-service/README.md(state/reactivity model, validation placement, load semantics, subscription flow including selector and reactive load).Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Release Notes
New Features
getStateSnapshot()method for retrieving detached state snapshots.Documentation